create temp CG for consistent VM snapshot for the VM which is span across multiple flexvolumes by rajiv-jain-netapp · Pull Request #74 · NetApp/cloudstack

rajiv-jain-netapp · 2026-07-02T13:05:43Z

Description

This PR...

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)
Build/CI
Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

… multiple volumes at storage

… at multiple flexvolumes

…ective checks by which delete snapshot would be handled from primary only

…t workflows

rajiv-jain-netapp · 2026-07-02T13:06:31Z

testing results and all the executed test cases added under previous PR #67

sandeeplocharla

Partially reviewed. Still need to go through a good portion of OntapVMSnapshotStrategy.java and SnapshotManagerImpl.java

Copilot

Pull request overview

This PR enhances NetApp ONTAP managed-storage snapshot handling by ensuring VM snapshots remain consistent when a VM’s volumes span multiple FlexVols, and by preventing managed PRIMARY (array-native) volume snapshots from being incorrectly archived/bookkept as secondary snapshots.

Changes:

Add a temporary ONTAP Consistency Group (CG) two-phase snapshot flow for VM snapshots spanning multiple FlexVols; keep a direct FlexVol snapshot fast-path for single-FlexVol VMs.
Ensure managed PRIMARY volume snapshots (e.g., ONTAP volume snapshots) remain on primary/array storage and do not use secondary-archive bookkeeping paths.
Extend ONTAP integration utilities/strategy to better separate “provisioning-time” aggregate validation from “operations-only” connectivity, and add CG REST client APIs + related unit tests.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java	Avoids archiving managed PRIMARY snapshots to secondary; refactors decision into `archiveSnapshotToSecondary`.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategy.java	Implements temporary CG orchestration for multi-FlexVol VM snapshots, plus single-FlexVol direct snapshot path and improved naming.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java	Adds operations-only connect mode, job polling helpers, and centralized FlexVol snapshot delete orchestration.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java	Adds snapshot naming helpers and an overload to connect with/without aggregate validation.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java	Adds SVM UUID support and CG/SFSR/delete polling constants.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java	Persists resolved SVM UUID into pool details when available.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SnapshotFeignClient.java	Adds ONTAP CG REST API endpoints (create/list CG, start/commit snapshot, delete CG).
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java	Delegates volume-snapshot delete/revert to strategy helpers; updates naming and makes delete more robust/idempotent.
plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/vmsnapshot/OntapVMSnapshotStrategyTest.java	Adds/updates unit tests for CG flow, naming, scope resolution, and avoids static utility connection issues.
plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java	Adds tests for operations-only connect, job polling helpers, and FlexVol snapshot delete orchestration.
plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java	Updates capabilities expectations to include revert support.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

sandeeplocharla · 2026-07-07T14:31:36Z

+        return null;
+    }
+
+    private String getStringField(Map<String, Object> record, String key) {


This can be in utils, instead.

sandeeplocharla · 2026-07-07T14:41:49Z

+    @RequestLine("POST /api/application/consistency-groups")
+    @Headers({"Authorization: {authHeader}", "Content-Type: application/json"})
+    JobResponse createConsistencyGroup(@Param("authHeader") String authHeader,
+                                       Map<String, Object> request);


Isn't it better to create a model for CG request, instead?

piyush5netapp · 2026-07-07T14:12:39Z

+
+            logger.info("takeVMSnapshot: [CG:Start] Starting phase-1 snapshot [{}] for temporary consistency group [{}]",
+                    snapshotNameBase, tempCgUuid);
+            cgSnapshotUuid = resolveStartedConsistencyGroupSnapshotUuid(snapshotClient, storageStrategy,


We can have null check for cgSnapshotUuid so that null is not passed to commitConsistencyGroupSnapshot for actual API call.

piyush5netapp · 2026-07-07T14:17:31Z

+     * Validates ONTAP cluster reachability, credentials, SVM state, protocol, and aggregate capacity
+     * for new FlexVol creation (primary pool provisioning).
+     */
    public boolean connect() {


if this method is always calling public boolean connect(boolean validateAggregatesForVolumeCreation) , what is this method intended for ? We can keep one of them or have default value for validateAggregatesForVolumeCreation if caller does not want to pass.

piyush5netapp · 2026-07-07T14:21:34Z

-            if (eligibleAggregates.isEmpty()) {
-                logger.error("No suitable aggregates found on SVM " + svmName + " for volume operations.");
-                throw new CloudRuntimeException("No suitable aggregates found on SVM " + svmName + " for volume operations.");
+            this.resolvedSvmUuid = svm.getUuid();


Since we are fetching svm uuid now , lets explicitly add uuid in queryParams

piyush5netapp · 2026-07-07T14:27:45Z

@@ -685,13 +715,116 @@ public Boolean jobPollForSuccess(String jobUUID, int maxRetries, int sleepTimeIn
                Thread.sleep(sleepTimeInMilliSecs);
            }
            if (jobResp == null || !jobResp.getState().equals(OntapStorageConstants.JOB_SUCCESS)) {


This condition will always be false now as while will take care of these conditions.

piyush5netapp · 2026-07-07T14:30:00Z

+            return;
+        }
+        Boolean success = jobPollForSuccess(response.getJob().getUuid(), maxRetries, pollIntervalMs);
+        if (!Boolean.TRUE.equals(success)) {


jobPollUntilSuccess always either throws CloudRuntimeException or returns a non-null Job . It never returns null. So jobPollForSuccess always returns true or throws. So this is in a way dead-code

piyush5netapp · 2026-07-07T14:37:31Z

+     * {@code snapshot_data_store} row must remain so volume-snapshot DELETE uses
+     * {@code StorageSystemSnapshotStrategy} and the primary datastore driver.</p>
+     */
+    private boolean isManagedPrimaryLocationSnapshot(StoragePool storagePool, CreateSnapshotPayload payload) {


Is this method limited to ONTAP snapshots? if yes, we should add our provider check as this will impact all the managed storage pool.

piyush5netapp · 2026-07-07T14:39:38Z

+    String buildTemporaryConsistencyGroupName(VMSnapshot vmSnapshot) {
+        return OntapStorageUtils.buildOntapSnapshotName(
+                OntapStorageConstants.ONTAP_TEMP_CG_PREFIX + vmSnapshot.getId(),
+                "cg" + vmSnapshot.getId());


vmSnapshot.getId will appear twice in temp cg name ?

piyush5netapp · 2026-07-07T14:49:05Z

-                StorageStrategy storageStrategy = OntapStorageUtils.getStrategyByStoragePoolDetails(poolDetails);
-                SnapshotFeignClient client = storageStrategy.getSnapshotFeignClient();
-                String authHeader = storageStrategy.getAuthHeader();
+                StorageStrategy storageStrategy = resolveStorageStrategy(poolDetails);


This and deleteFlexVolSnapshotForCloudStackVolume can throw exception which will cause line vmSnapshotDetailsDao.remove(detailVO.getId()); to skip. That will cause snapshot to be in still present in DB. May be we can move vmSnapshotDetailsDao.remove(detailVO.getId()); in finally block with these 2 on try

suryag1201 · 2026-07-07T14:51:24Z

+                if (lunDetail.getValue() == null) {
                    throw new CloudRuntimeException("LUN UUID not found for iSCSI volume " + volumeVO.getId());
                }
+                lunUuid = lunDetail.getValue();


no need of this extra lunUuid variable assignment with temp lunUUID var, can be done directly also

suryag1201 · 2026-07-07T14:51:49Z

-            name = StringUtils.left(volumeName, volumeName.length() - trimRequired) + "-" + snapshotUuid;
-        }
-        return name;
+    private String buildSnapshotName(String cloudStackSnapshotName, long snapshotId) {


as discussed, we have to check for two cloudstack using same ontap.

suryag1201 · 2026-07-07T14:52:28Z

-        }
-        return name;
+    private String buildSnapshotName(String cloudStackSnapshotName, long snapshotId) {
+        return OntapStorageUtils.buildOntapSnapshotName(cloudStackSnapshotName, "cs" + snapshotId);


we can move this cs to constant
Also, for better readability we can use - (hyphen)
Please add example for snapshot name

suryag1201 · 2026-07-07T14:52:58Z

+    @RequestLine("POST /api/application/consistency-groups")
+    @Headers({"Authorization: {authHeader}", "Content-Type: application/json"})
+    JobResponse createConsistencyGroup(@Param("authHeader") String authHeader,
+                                       Map<String, Object> request);


to create CG, request should be model type why map?

suryag1201 · 2026-07-07T14:54:17Z

+     */
+    @RequestLine("GET /api/application/consistency-groups")
+    @Headers({"Authorization: {authHeader}"})
+    OntapResponse<Map<String, Object>> getConsistencyGroups(@Param("authHeader") String authHeader,


the return type should be OntapResponse of type ConsistencyGroup Object

suryag1201 · 2026-07-07T14:54:36Z

+     */
+    @RequestLine("POST /api/application/consistency-groups/{cgUuid}/snapshots")
+    @Headers({"Authorization: {authHeader}", "Content-Type: application/json"})
+    JobResponse createConsistencyGroupSnapshot(@Param("authHeader") String authHeader,


same here for request

suryag1201 · 2026-07-07T14:54:47Z

+     */
+    @RequestLine("GET /api/application/consistency-groups/{cgUuid}/snapshots")
+    @Headers({"Authorization: {authHeader}"})
+    OntapResponse<Map<String, Object>> getConsistencyGroupSnapshots(@Param("authHeader") String authHeader,


same here for return type

suryag1201 · 2026-07-07T14:55:37Z

-                            groupInfo.volumeIds);
+                // CG orchestration is only required when VM disks span multiple FlexVols.
+                // A single FlexVol already provides atomic capture for all volumes on that FlexVol.
+                if (flexVolGroups.size() > 1) {


can we also test, VM span across mutiple storage pool where svms are also diff

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

            if (ProtocolType.ISCSI.name().equalsIgnoreCase(protocol)) {
                VolumeDetailVO lunDetail = volumeDetailsDao.findDetail(volumeVO.getId(), OntapStorageConstants.LUN_DOT_UUID);
-                String lunUUID = lunDetail != null ? lunDetail.getValue() : null;
-                if (lunUUID == null) {
+                if (lunDetail.getValue() == null) {
                    throw new CloudRuntimeException("LUN UUID not found for iSCSI volume " + volumeVO.getId());
                }
+                lunUuid = lunDetail.getValue();


+            Aggregate aggrResp = aggregateFeignClient.getAggregateByUUID(authHeader, aggr.getUuid());
+            if (aggrResp == null) {
+                logger.warn("Aggregate details response is null for aggregate " + aggr.getName() + ". Skipping.");
+                break;
+            }


+    /**
+     * Single GET attempt: try to match by name,
+     * then fall back to listing all CG snapshots in this group (And it would be one
+     * always since workflow is keep deleting the CG).
+     */


rajiv-jain-netapp added 8 commits June 23, 2026 19:06

CSTACKEX:200 - added temp CG logic for VM snapshots if VM span across…

3efaa7a

… multiple volumes at storage

CSTACKEX-200: added consistency for the VM level snapshots if CSV are…

4ec7c18

… at multiple flexvolumes

CSTACKEX-200: ONTAP will have snapshot always on primary, adding resp…

4b482df

…ective checks by which delete snapshot would be handled from primary only

CSTACKEX-200: skip aggr space validations for existing volume operations

2724dbb

CSTACKEX-200: CG create must have SVM name and respective validations

4d1377c

CSTACKEX-200: add provisioning options

f72d5cf

CSTACKEX-200 race conditions needs to be handled while taking snapsho…

5d9639e

…t workflows

CSTACKEX-200: state is not a valid field for CG

26f20d7

rajiv-jain-netapp requested review from piyush5netapp, sandeeplocharla and suryag1201 as code owners July 2, 2026 13:05

sandeeplocharla reviewed Jul 6, 2026

View reviewed changes

rajiv-jain-netapp requested a review from Copilot July 7, 2026 09:06

Copilot started reviewing on behalf of rajiv-jain-netapp July 7, 2026 09:06 View session

CSTACKEX-200: incorporate one round of review comments

0d3f4b7

Copilot AI reviewed Jul 7, 2026

View reviewed changes

rajiv-jain-netapp added 2 commits July 7, 2026 16:59

CSTACKEX-200: incorporate copilot comments

605a878

Merge branch 'main' into feature/CSTACKEX-200

61f8105

sandeeplocharla reviewed Jul 7, 2026

View reviewed changes

piyush5netapp reviewed Jul 7, 2026

View reviewed changes

suryag1201 reviewed Jul 7, 2026

View reviewed changes

CSTACKEX-200: fix for the issue added due to merge

e1c44f5

Copilot AI review requested due to automatic review settings July 8, 2026 07:04

Copilot started reviewing on behalf of rajiv-jain-netapp July 8, 2026 07:04 View session

Copilot AI reviewed Jul 8, 2026

View reviewed changes

Uh oh!

Conversation

rajiv-jain-netapp commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Uh oh!

rajiv-jain-netapp commented Jul 2, 2026

Uh oh!

sandeeplocharla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

piyush5netapp Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

rajiv-jain-netapp commented Jul 2, 2026 •

edited

Loading

piyush5netapp Jul 7, 2026 •

edited

Loading